-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix calculation for slot containing offset #736
Conversation
Fixes issue dotnet#605 Add logic to not return zero-length nodes, since they never contain their offset.
@pharring might want to comment. |
We need the same fix for VB. |
@@ -53,7 +53,19 @@ public override int FindSlotIndexContainingOffset(int offset) | |||
{ | |||
Debug.Assert(offset >= 0 && offset < FullWidth); | |||
int idx = _childOffsets.BinarySearch(offset); | |||
return idx >= 0 ? idx : (~idx - 1); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wondered about this case when I wrote it. I originally had a hand-coded binary search that avoided this problem, but I was 'encouraged' to change it to Array.BinarySearch in review.
What we really need is an "upper_bound" equivalent (Array.BinarySearch gives the lower bound).
Note that this BinarySearch is already an extension method over int[]. We could introduce a "UpperBoundBinarySearch" method. That would be nice to avoid code duplication (both C# and VB need the same fix).
Here is an "upper_bound" binary search algorithm. The difference from Array.BinarySearch and the existing BinarySearch extension method is subtle, but significant: We don't check for equality in the middle. We keep halving the search space until there's only a single value. Note that, since it's an upper bound, it can return a value equal to array.Length (i.e. off the end of the array).
With this, I believe FindSlotIndexContainingOffset can be implemented like this:
|
@mattwar Let me know if you want me to take care of this for you. (I feel bad since I introduced the bug in the first place) |
@pharring please review the newest changes |
👍 please merge at your earliest convenience. |
Fix calculation for slot containing offset
Fixes issue #605
Add logic to not return zero-length nodes, since they never contain their offset.